Skip to content

Don't report the RPC shutdown force-kill as a crash - #8537

Draft
jkschneider wants to merge 1 commit into
mainfrom
rpc-shutdown-force-kill
Draft

Don't report the RPC shutdown force-kill as a crash#8537
jkschneider wants to merge 1 commit into
mainfrom
rpc-shutdown-force-kill

Conversation

@jkschneider

@jkschneider jkschneider commented Aug 17, 2026

Copy link
Copy Markdown
Member

RewriteRpcProcess.shutdown() force-kills the RPC subprocess, then reports its own kill as a crash — failing commands that had already fully succeeded:

java.lang.RuntimeException: Rewrite RPC process crashed with exit code: 137
  org.openrewrite.rpc.RewriteRpcProcess.shutdown(RewriteRpcProcess.java:261)
  org.openrewrite.javascript.rpc.JavaScriptRewriteRpc.shutdown(JavaScriptRewriteRpc.java:101)
  org.openrewrite.rpc.RewriteRpcProcessManager.shutdown(RewriteRpcProcessManager.java:69)
  io.moderne.cli.commands.config.recipes.Npm$Install.onComplete(Npm.java:152)

Cause

shutdown() sends SIGTERM, waits 5 seconds, and escalates to destroyForcibly() if the subprocess hasn't exited. It then checked the exit code against an allowlist of {0, 1, 143} — which covers the graceful SIGTERM path but not 137 (128 + 9), the exit code its own SIGKILL had just produced two lines earlier.

It only fires when the subprocess needs more than 5 seconds to handle SIGTERM, so it tracks machine load rather than anything about the command. That is what makes it expensive: it presents as a random crash in an RPC process that is in fact healthy, so the natural next step is to investigate the subprocess.

Fix

Move the exit-code check inside the graceful branch instead of running it after both branches. The force-kill branch no longer inspects an exit code it caused.

This is deliberately structural rather than adding 137 to the allowlist: a genuine external SIGKILL — an OOM-killer kill arriving within the grace period — is a real failure, and allowlisting 137 would have swallowed it. It still throws.

Three smaller things in the same method:

  • The grace period is now configurable via setShutdownGracePeriod(Duration), defaulting to the existing 5s.
  • The message names what happened. Rewrite RPC process exited with code N in response to shutdown, plus a stderr-log pointer when one is configured, instead of asserting a "crash" the caller cannot distinguish from a timeout. For 137 specifically it notes the SIGKILL came from outside Rewrite — which is now the only way to reach that message.
  • The exception is thrown after the stderr drain join, not through it. That join exists so the parent-side log handle is released before shutdown() returns; throwing past it leaked the handle on Windows. That mattered little while the throw was spurious and matters now that it is rare and real.

Tests

Two tests in RewriteRpcProcessTest, both forking a JVM and synchronising on a readiness marker file so they cannot silently degrade into asserting about an ordinary JVM exit:

  • shutdownDoesNotReportItsOwnForceKillAsFailure — the child blocks in a shutdown hook so it cannot exit on SIGTERM. Asserts shutdown() doesn't throw and that the child's exit code was actually 137, so the force-kill path is provably the one exercised.
  • shutdownStillReportsAnExitCodeTheSubprocessChose — the child halt(3)s from its shutdown hook. Asserts the throw still happens, carries the stderr log path, and that the drain thread was joined before it escaped.

Both are @DisabledOnOs(WINDOWS): there Process.destroy() maps to TerminateProcess, which a child can neither delay nor intercept, so neither scenario is constructible.

Reintroducing the bug against these tests reproduces the report verbatim (RuntimeException: Rewrite RPC process crashed with exit code: 137); reverting it turns them green. :rewrite-core:check passes.

  • Reported in moderneinc/moderne-saas#1895.

`shutdown()` SIGTERMs the subprocess, waits 5s, and escalates to
`destroyForcibly()` if it hasn't exited. It then checked the exit code
against an allowlist of {0, 1, 143} — which covers the graceful SIGTERM
path but not 137, the exit code its own SIGKILL had just produced. A
command whose work had fully completed failed with "Rewrite RPC process
crashed with exit code: 137".

It fired only when the subprocess needed more than 5s to handle SIGTERM,
so it tracked machine load rather than anything about the command, and
presented as a random crash in a healthy RPC process.

Move the exit-code check inside the graceful branch instead of running it
after both. The force-kill branch no longer inspects an exit code it
caused, so a genuine external SIGKILL — an OOM killer kill arriving within
the grace period — is still surfaced, which allowlisting 137 would have
swallowed.

Also:

- The grace period is now settable via `setShutdownGracePeriod(Duration)`,
  defaulting to the existing 5s.
- The message names what happened ("exited with code N in response to
  shutdown") and points at the stderr log when one is configured, rather
  than asserting a crash the caller cannot distinguish from a timeout.
- The exception is held in a local and thrown after the stderr drain join
  rather than through it. That join exists to release the parent-side log
  handle before `shutdown()` returns; skipping it leaked the handle on
  Windows, which mattered little while the throw was spurious and matters
  now that it is rare and real.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

1 participant